fix: make utils.tqdm thread-safe #3286
Conversation
Signed-off-by: Michele Dolfi <[email protected]>
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
There was a problem hiding this comment.
Thanks @dolfim-ibm for the PR! I left a comment. The test test_progress_bar_respects_group is currently failing because the new tqdm class overrides the original one and drops the custom init that handled the name= kwarg. this can fix it by re adding the original __init__.
src/huggingface_hub/utils/tqdm.py
Outdated
| name = kwargs.pop("name", None) # do not pass `name` to `tqdm` | ||
| if are_progress_bars_disabled(name): |
There was a problem hiding this comment.
is there a reason why dropping the original __init__ and __delattr__?
even when keeping the original __delattr__, del tqdm._lock will call SafeDelLockMeta.__delattr__ (which safeguards the class level race only).
There was a problem hiding this comment.
you are definitely correct, thanks for the suggestions. now the PR is updated.
Signed-off-by: Michele Dolfi <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
|
Hey @dolfim-ibm @hanouticelina , moving the convo from #3285 (comment) to this PR directly. I can confirm that I am able to reproduce the issue with your script from #3285 (comment) but without disabling the progress bars. I think disabling the progress bars is what fixes the issue in the reproducible script, not the content of this PR. More precisely, when I run it a few times I get different behaviors:
@dolfim-ibm can you confirm to me that you can reproduce the same behavior as mine, even with the changes from your PR? If so, wanna give it a look again on how to fix it? :) |
|
This is my setup:
I'm running the code in #3285 as for i in {1..20}; do python run_threaded.py; doneMy results:
So, I think the PR is currently addressing one of the scenario, but it still fails on the other one. |
|
Thanks for the extensive test. I agree, this seems to fix the case "concurrency issues + progress bars are disabled" (still wonder why to be honest 😄). But before moving forward / merging this PR, I would really prefer to have a fix for the main use case which is "concurrency issues + progress bars are enabled", which is the case most people will fall into. Disabling progress bars is quite a niche option at this stage. If not a fix, at least an idea of what's going wrong under the hood 😕 |
Signed-off-by: Michele Dolfi <[email protected]>
|
My last tests with 2dff573 seems to work also for the case with progressbars enabled. The idea behind it is to use a "backup" |
Applied the same patch suggested in huggingface/datasets#7661.
Resolves #3285